Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving the reusability and maintainability in ScrollbarExt.kt file and Fix the scroll bar thumb jumping a little bit due to lack of smothness on interpolatedIndex #1550

Conversation

rodrigodiasferreira
Copy link

@rodrigodiasferreira rodrigodiasferreira commented Jul 14, 2024

What I have done and why

Improved the reusability and maintainability of the file ScrollbarExt.kt file by implementing a generic: <LazyState : ScrollableState, LazyStateItem> LazyState.genericScrollbarState, and reducing the repetitive logic and code.

Related issue:

@rodrigodiasferreira rodrigodiasferreira changed the title Improving the reusability and maintainability in ScrollbarExt.kt file #1549 Improving the reusability and maintainability in ScrollbarExt.kt file Jul 14, 2024
@yongsuk44
Copy link
Contributor

Sometimes, I believe that dividing similar logic into multiple extension functions is done to reduce the learning curve of the logic.

When I look at the refactored code, it seems that jumping from the extension function to genericScrollbarState and then jumping back to the extension function to check arguments could occur. This might lower the readability for developers who are learning the code, and I am concerned that it could increase the learning curve. 🥲

@rodrigodiasferreira
Copy link
Author

Hi @yongsuk44, thanks for your comment making this discussion richer.
I disagree. When I was studying the Now In Android project, I had to compare the 3 functions in order to check what they had in common, and then I figure it out, it is pretty much the same logic, but with the differences that each type brings: LazyListState, LazyGridState, LazyStaggeredGridState. This refactoring unify the logic into a unique function, and makes very clear that they perform the same logic, which makes easier for study in my opinion, because you don't need to read the 3 different functions to realize that they are the same logic.

Moreover, if there will be some need to fix something in that logic in the future, it won't be necessary to change on the 3 different functions, only in one place, which improves also the maintainability. Besides one could even forget to change on these other places, which would cause different behavior for the scroll bar for the different type.

In summay, in my opinion, this refactoring is a good improvement for Now In Android code base in aspects of study, reusability and maintainability.

@anhtuannd
Copy link

anhtuannd commented Jul 15, 2024

Hi,
I think LazyStaggeredGridState's ScrollBar needs some improvement on how we calculate thumb size and position, as seen in #1526

So merging them into once might not be the best approach.

@rodrigodiasferreira
Copy link
Author

Hi @anhtuannd , thanks for raising this concern.
As I commented on the issue: #1526, I believe I've fixed that issue on the PR: #1553, and the solution was done in a common place, so the unifying done on this PR, won't affect the fix of that issue. Please check when you have some time, and let me know, if you agree...

@rodrigodiasferreira rodrigodiasferreira changed the title Improving the reusability and maintainability in ScrollbarExt.kt file Improving the reusability and maintainability in ScrollbarExt.kt file and Fix the scroll bar thumb jumping a little bit due to lack of smothness on interpolatedIndex Jul 15, 2024
…s on interpolatedIndex

Signed-off-by: Rodrigo Dias Ferreira <[email protected]>
@rodrigodiasferreira rodrigodiasferreira force-pushed the scrollbarext-improvement branch from 2fd6bb4 to 31b77d5 Compare July 15, 2024 22:25
@tunjid
Copy link
Contributor

tunjid commented Aug 10, 2024

Hi hi! Original author of the Scrollbar here. It used to have a generic abstraction, but we split up out for readability.

You can see the original PR where the generic code was split up favoring readability over copy pasting here: #947 (comment)

@rodrigodiasferreira
Copy link
Author

Hi TJ / @tunjid , how are you?
I watched several videos that you recorded on the Android Developers site.
It's an honor to meet you!

I understand what you are saying, but on my opinion I still think that the generic abstraction eliminates some duplicated logic and improves the maintainability. And also I prefer to read the more concise code, which I know for sure that the parts that use the same generic abstraction, they are the same, no need to compare the code and checking that they do the same thing.

But since you guys made this decision consciously, I will open a new PR with the fix on the commit: 31b77d5, and close this one.

Thanks a lot for the feedback. :-)

@rodrigodiasferreira
Copy link
Author

Hi @tunjid,

I've uploaded the commit: 31b77d5 into the PR: #1553. So the fix related to issue: #1526 is concentrated on the same PR.
I will close this one.

Once again, thanks for the feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants